add option zimbra in ldap app #4567
Conversation
|
@johuder33, thanks for your PR! By analyzing the history of the files in this pull request, we identified @blizzz, @LukasReschke and @leo-b to be potential reviewers. |
|
@johuder33 thanks for your contribution! May I ask you to look into:
|
|
@blizzz Ok, I will modify the code and explain it too. |
|
I just had a quick look. Wouldn't it be nice to have a general approach for Group Membership with E-Mails. There are some other ObjectClasses which work the same way only a different attribute. We could make group membership attributes configurable with a switch weather it contains a email address or a user id. |
|
@go2sh Thank you for your suggestion, right now I working on this, I hope for the next days I can make your suggestion, it would be nice too. I will considere your comment for the next PR. 👍 |
|
If you need some help with it, feel free to ask ;) |
johuder33
force-pushed
the
add-ability-zimbra-ldap
branch
from
dd5fe48 to
5c6a7fc
Compare
Codecov Report
@@ Coverage Diff @@
## master #4567 +/- ##
============================================
- Coverage 54.28% 54.26% -0.03%
- Complexity 22099 22106 +7
============================================
Files 1361 1361
Lines 84708 84739 +31
Branches 1322 1322
============================================
- Hits 45983 45980 -3
- Misses 38725 38759 +34
|
…ra accounts Signed-off-by: Juorder Gonzalez quiñonez <[email protected]>
johuder33
force-pushed
the
add-ability-zimbra-ldap
branch
from
5c6a7fc to
da20a58
Compare
|
@blizzz the changes are done, please add me to the group @nextcloud/ldap, there's not problem with that. Any doubt you have please let me know. |
FYI looks like your git config is not set to your real email address 😉 That's also why GitHub doesn't recognize it's your commit. |
|
|
||
| if(strtolower($this->access->connection->ldapGroupMemberAssocAttr) === 'zimbramailforwardingaddress') { | ||
| // array containing domains | ||
| $dns = array(); |
There was a problem hiding this comment.
these days you can use short notation $dns = []; :)
| if(strtolower($this->access->connection->ldapGroupMemberAssocAttr) === 'zimbramailforwardingaddress') { | ||
| // array containing domains | ||
| $dns = array(); | ||
| // loop througth array members |
| $dns = array(); | ||
| // loop througth array members | ||
| foreach($members as $mailOfMember) { | ||
| // split the email of each member eg. [email protected] |
There was a problem hiding this comment.
intendentation here and the following four lines appears wrong
| // array containing domains | ||
| $dns = array(); | ||
| // loop througth array members | ||
| foreach($members as $mailOfMember) { |
There was a problem hiding this comment.
It seems this can take a lot of time on big groups with many members. Do you think it is possible to use the provided $uid attribute and attempt to look up the user directly?
| ); | ||
| if ($userMatch !== false) { | ||
| // match found so this user is in this group | ||
| $pos = strpos($dynamicGroup['dn'][0], ','); |
There was a problem hiding this comment.
did you test this against groups having a , in their cn?
| // we need to extract the group name | ||
| // so we extracted from cn=myGroup,ou=people,dc=domain,dc=com | ||
| // we extract from position 3 to 7 in the string above, so we have: myGroup | ||
| $groups[] = $membershipGroup; |
There was a problem hiding this comment.
This should be dealt with $this->access->dn2groupname, because we rely on mappings here and avoid name collisions (e.g. with groups from other backends). Adding the group just like this won't have it mapped and further operations against this group might not work.
| $member = $memberParts[0]; | ||
|
|
||
| // replace all '%uid' with the value of $member and get the filter query | ||
| $filter = $this->access->combineFilterWidthAnd(array( |
There was a problem hiding this comment.
Typo in method name, it should be combineFilterWithAnd
|
@johuder33 thanks a lot! Please see my review comments, important points are a typo in method name, one worry about group mapping and a question on speeding up one part. I also invited you to the group. |
MorrisJobke
added
2. developing
|
@johuder33 any update? i move it 14 due to feature freeze. |
|
No feedback for 10 months -> closing 😢 |
7 participants
this PR allow us to connect zimbra LDAP with the user_ldap app, now zimbra users can login with their zimbra accounts to Nextcloud.